-
Couldn't load subscription status.
- Fork 117
Update wordpress span Name to include low cardinal target #414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update wordpress span Name to include low cardinal target #414
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #414 +/- ##
=========================================
Coverage 83.07% 83.07%
Complexity 1530 1530
=========================================
Files 97 97
Lines 6115 6115
=========================================
Hits 5080 5080
Misses 1035 1035 Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
This looks pretty good to me, is it ready for review? |
|
@brettmc , yes it is ready for review. DO you want me to open the PRs for the same change for all auto instrumented libraries in separate PR's or in this one only ? |
|
I tested it in my local environment, works for me. ✅ I have also been considering an alternative value. We could have changed the name in the shutdown function. But there is not really a good value in WordPress. For <?php
register_shutdown_function(function () use ($span) {
...
global $wp;
echo $wp->query_string; // ‘pagename=example’
echo $wp->request; // ‘example’
echo $wp->matched_rule; // ‘(.?.+?)(?:/([0-9]+))?/?$’
echo $wp->matched_query; // ‘pagename=example&page=’
...
$span->updateName( ... )`
|
Fixes open-telemetry/opentelemetry-php#1671
However this change is required for multiple instrumented libraries.